perf: import pandas on first row conversion instead of in connect() - #1049

🔵 Low — The child script uses exit code 1 to mean "pandas was imported", but Python also exits with 1 on any uncaught exception. If import {module_name} raises (an ImportError from a missing optional dependency, or a NameError introduced by a refactor), the child exits 1. The returncode not in (0, 1) skip branch then never runs, and the test fails with the misleading message importing X imported pandas.

The guard does still fail rather than falsely pass, but the skip path is effectively dead code and the failure message points at the wrong cause. Wrap the import in try/except and use distinct sentinels, for example 0 = clean, 10 = pandas loaded, 20 = import error, which you then skip on. That matches the pattern the repo adopted after PR #907.

Fixed in 4041f33: the child wraps the import in try/except and exits with distinct sentinels (10 clean, 11 pandas loaded, 12 import failed → skip), so an import error is skipped with its traceback and any other exit fails with the child's stderr.

Thanks for the reply. The checked-out head doesn't contain this fix yet. tests/unit/test_lazy_pandas_import.py still runs import {module_name} with no try/except, still exits with 1 if "pandas" in sys.modules else 0, and still checks returncode not in (0, 1) before skipping. None of the 10/11/12 sentinels you described are in the file, so commit 4041f33 probably hasn't been pushed to this branch, or it was pushed somewhere else. When that commit lands on the PR head, the next review pass will check it.

🔵 Low — With this change, a process using _disable_pandas=True should never import pandas at all. That is a new property, and nothing pins it. The new tests check the connect path and the pandas conversion path, but not the as_py branch. If someone later moves import pandas above the disable_pandas check (for example to the top of _convert_arrow_table), every test here still passes and those users quietly pay the import again.

Suggested fix: add a sibling subprocess test that sets rs.connection.disable_pandas = True, calls ResultSet._convert_arrow_table, and asserts "pandas" not in sys.modules and the expected rows.

Added in 916638b: test_disable_pandas_never_imports_pandas. Writing it showed the existing conversion test was weaker than it looked: building the test table from Python lists makes pyarrow import pandas itself (its pandas shim), so "pandas in sys.modules" held before the conversion ran. Both tests now read the table from IPC bytes, check pandas isn't loaded beforehand, and assert the rows; both fail on main.

I couldn't find the 916638b changes at head. tests/unit/test_lazy_pandas_import.py still has only test_connect_path_does_not_import_pandas and test_row_conversion_imports_pandas_on_first_use. There's no test_disable_pandas_never_imports_pandas, and the conversion test still builds its table with pyarrow.table({...}) from Python lists, not from IPC bytes. You may not have pushed the commit yet, or it went to a different branch. Your point about pyarrow's pandas shim matters: if building the table imports pandas, then assert "pandas" in sys.modules passes whether or not the conversion caused the import. So the IPC-based version is worth landing. I'll check again once the commit shows up on this PR's head.

🔵 Low — _run calls subprocess.run with no timeout. If a child interpreter hangs during import (for example, a kernel wheel that blocks at load), the unit job stalls until CI's global timeout, with no useful failure message. Adding a bounded timeout= (e.g. 120 s) would turn a hang into a clear TimeoutExpired failure.

Added timeout=120 to _run in 916638b.

Thanks for picking this up. The head checkout still doesn't have the timeout, though. _run in tests/unit/test_lazy_pandas_import.py calls subprocess.run with only capture_output=True and text=True. There's no timeout=120, so commit 916638b either wasn't pushed to this branch or doesn't include that change. Could you check that the commit is on the PR branch? Once timeout= shows up in _run, this is resolved.